test: add E2E tests for bru.variables, bru.environment, bru.globals APIs#7888
test: add E2E tests for bru.variables, bru.environment, bru.globals APIs#7888sanish-bruno wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces legacy variable helpers with namespaced Bruno PropertyList APIs: introduces VariableList and exposes ChangesNamespaced Variable APIs, Runtime, Shim, Translator, and Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru (1)
14-16: Use suite-scoped key names to reduce cross-run collisions.These tests use generic global keys. In persisted/shared runs, collisions can make the missing-key assertion environment-dependent.
♻️ Suggested key hardening
- bru.globals.set("testGlobalVar", "global-hello-123"); - bru.globals.set("testGlobalObj", { scope: "global", active: true }); + bru.globals.set("__bru_globals_get_set_var__", "global-hello-123"); + bru.globals.set("__bru_globals_get_set_obj__", { scope: "global", active: true }); - expect(bru.globals.get("testGlobalVar")).to.equal("global-hello-123"); + expect(bru.globals.get("__bru_globals_get_set_var__")).to.equal("global-hello-123"); - const obj = bru.globals.get("testGlobalObj"); + const obj = bru.globals.get("__bru_globals_get_set_obj__"); - expect(bru.globals.get("nonexistent")).to.be.undefined; + expect(bru.globals.get("__bru_globals_get_set_missing__")).to.be.undefined; - bru.globals.set("testGlobalVar", "updated-456"); - expect(bru.globals.get("testGlobalVar")).to.equal("updated-456"); + bru.globals.set("__bru_globals_get_set_var__", "updated-456"); + expect(bru.globals.get("__bru_globals_get_set_var__")).to.equal("updated-456");As per coding guidelines "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
Also applies to: 29-31, 34-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru` around lines 14 - 16, Replace the generic global keys with suite-scoped unique names to avoid cross-run collisions: change the bru.globals.set calls that use "testGlobalVar" and "testGlobalObj" to deterministic, suite-prefixed keys (for example use a constant like "get-set::testGlobalVar" and "get-set::testGlobalObj" or include the test/suite name as a prefix) and update any matching bru.globals.get/clear references elsewhere in this file (the other instances around the same test) to use the new names so all set/get/clear operations remain consistent.packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru (1)
14-16: Use test-scoped global keys to reduce collision risk.Consider prefixing keys (e.g.
__bru_test_globals_iteration_gA) so this request is safer in shared/global environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru` around lines 14 - 16, Replace the plain global keys "gA", "gB", "gC" with test-scoped, prefixed keys to avoid collisions in shared environments; update the bru.globals.set calls to use a unique prefix such as "__bru_test_globals_iteration_" (e.g. "__bru_test_globals_iteration_gA") for each key and ensure any reads of these globals elsewhere in this test reference the same prefixed names (look for bru.globals.set and bru.globals.get usages in this iteration test).packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js (1)
108-116: Add a fail-fast guard for invalidsyncWriteMethodsentries.A typo in method config currently surfaces as an opaque runtime error. A small guard gives clearer diagnostics.
Proposed hardening
for (const methodName of syncWriteMethods) { + if (typeof nativeList[methodName] !== 'function') { + throw new Error(`createPropertyListBridge: "${methodName}" is not a function on nativeList`); + } const fn = vm.newFunction(methodName, (...vmArgs) => { const args = vmArgs.map((a) => vm.dump(a)); nativeList[methodName](...args); return vm.undefined; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js` around lines 108 - 116, The loop that creates sync write wrappers using syncWriteMethods can call undefined native methods if the config has a typo; update the block that iterates syncWriteMethods (the for...of creating functions via vm.newFunction and calling nativeList[methodName]) to validate each methodName up front: if typeof nativeList[methodName] !== "function" throw a clear Error mentioning the invalid methodName and context (e.g., property-list-bridge syncWriteMethods for targetObj), otherwise proceed to create the vm.newFunction wrapper that calls nativeList[methodName]; this ensures a fail-fast, descriptive error instead of an opaque runtime crash.packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru (1)
14-15: Use namespaced global keys to reduce cross-test interference.Because globals are shared and teardown is currently unavailable, generic keys like
gX/gYcan collide with other scripts. Prefer unique test-scoped keys (for example,__cr_globals_readMethods_gX).As per coding guidelines "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru` around lines 14 - 15, The globals use generic keys ("gX" and "gY") which can collide across tests; change calls to bru.globals.set that use "gX" and "gY" to use test-scoped, namespaced keys (e.g. prefix with "__cr_globals_readMethods_") so the keys are unique and reduce cross-test interference while keeping the same values and usage in any corresponding bru.globals.get calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-js/src/bru.js`:
- Around line 90-99: The onSet handler in bru.js currently checks if
(this.persistentEnvVariables[key]) before deleting, which fails for falsy string
values like '' and leaves stale persisted entries; update the deletion check in
the onSet callback to test for the key's presence (e.g., use
Object.prototype.hasOwnProperty.call(this.persistentEnvVariables, key) or the
`in` operator) so that any existing persisted key (even if its value is an empty
string) is removed from this.persistentEnvVariables.
In `@packages/bruno-js/src/variable-list.js`:
- Around line 32-33: The get() method currently reads directly from
_variablesObj and can return keys that should be excluded by options.filterKeys;
update get() to respect the same exclusion logic used by has()/all()/toObject()
by consulting the filtered view instead of _variablesObj directly — e.g., check
the filterKeys set (or reuse this.toObject()/this.all()) and return undefined if
the requested key is in filterKeys, otherwise return the value from the filtered
object; ensure any internal references to _variablesObj in get() are replaced
with the filtered lookup so behavior is consistent across get(), has(), all(),
and toObject().
- Around line 71-79: The set method on VariableList directly assigns keys after
calling `#validateKey`, but the current regex allows prototype-related reserved
keys like "__proto__", "constructor", and "prototype", enabling prototype
pollution; update the key validation logic in VariableList.#validateKey (and the
shared validateKey in bru.js) to explicitly reject reserved property names (at
least "__proto__", "prototype", "constructor") and throw an Error when
encountered instead of allowing assignment, so variables/environment/globals
cannot mutate prototypes; keep the existing regex check but add a reserved-key
blacklist check and reuse the same guarded validation in all places that call
validateKey.
---
Nitpick comments:
In `@packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js`:
- Around line 108-116: The loop that creates sync write wrappers using
syncWriteMethods can call undefined native methods if the config has a typo;
update the block that iterates syncWriteMethods (the for...of creating functions
via vm.newFunction and calling nativeList[methodName]) to validate each
methodName up front: if typeof nativeList[methodName] !== "function" throw a
clear Error mentioning the invalid methodName and context (e.g.,
property-list-bridge syncWriteMethods for targetObj), otherwise proceed to
create the vm.newFunction wrapper that calls nativeList[methodName]; this
ensures a fail-fast, descriptive error instead of an opaque runtime crash.
In `@packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru`:
- Around line 14-16: Replace the generic global keys with suite-scoped unique
names to avoid cross-run collisions: change the bru.globals.set calls that use
"testGlobalVar" and "testGlobalObj" to deterministic, suite-prefixed keys (for
example use a constant like "get-set::testGlobalVar" and
"get-set::testGlobalObj" or include the test/suite name as a prefix) and update
any matching bru.globals.get/clear references elsewhere in this file (the other
instances around the same test) to use the new names so all set/get/clear
operations remain consistent.
In `@packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru`:
- Around line 14-16: Replace the plain global keys "gA", "gB", "gC" with
test-scoped, prefixed keys to avoid collisions in shared environments; update
the bru.globals.set calls to use a unique prefix such as
"__bru_test_globals_iteration_" (e.g. "__bru_test_globals_iteration_gA") for
each key and ensure any reads of these globals elsewhere in this test reference
the same prefixed names (look for bru.globals.set and bru.globals.get usages in
this iteration test).
In `@packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru`:
- Around line 14-15: The globals use generic keys ("gX" and "gY") which can
collide across tests; change calls to bru.globals.set that use "gX" and "gY" to
use test-scoped, namespaced keys (e.g. prefix with "__cr_globals_readMethods_")
so the keys are unique and reduce cross-test interference while keeping the same
values and usage in any corresponding bru.globals.get calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d307d3e-d66e-4916-8943-579d466874c8
📒 Files selected for processing (44)
packages/bruno-converters/src/postman/postman-translations.jspackages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-converters/tests/postman/postman-translations/postman-comments.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-edge-cases.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-variables.spec.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/combined.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/environment.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/legacy-tests-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/multiline-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/postman-references.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/response.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/testing-framework.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variable-chaining.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variables.test.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.jspackages/bruno-js/src/variable-list.jspackages/bruno-js/tests/variable-list.spec.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
b4cf838 to
de50718
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-tests/collection/scripting/api/bru/environment/iteration.bru`:
- Around line 45-48: The test currently calls Array.reduce on the local items
variable (created by bru.environment.filter), so it never exercises the new
PropertyList API; change the test to call bru.environment.reduce directly (use
the same reducer function and initial value), e.g. replace the items.reduce(...)
call with bru.environment.reduce(function(acc, i) { return acc +
Number(i.value); }, 0) inside the "reduce() should accumulate values" test so
bru.environment.reduce is executed and covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f97a998c-d6e6-443e-ad8b-7c306e360f18
📒 Files selected for processing (27)
packages/bruno-app/src/utils/collections/index.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
✅ Files skipped from review due to trivial changes (17)
- tests/scripting/bru-api/environment/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/variables/folder.bru
- tests/scripting/bru-api/variables/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/environment/folder.bru
- tests/scripting/bru-api/variables/variables.spec.ts
- packages/bruno-tests/collection/scripting/api/bru/globals/has.bru
- tests/scripting/bru-api/globals/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/folder.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru
- tests/scripting/bru-api/globals/globals.spec.ts
- tests/scripting/bru-api/variables/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/environment/readMethods.bru
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/scripting/bru-api/globals/init-user-data/collection-security.json
- tests/scripting/bru-api/environment/init-user-data/preferences.json
- tests/scripting/bru-api/environment/environment.spec.ts
- packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/iteration.bru
- packages/bruno-js/src/bru.js
- packages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.bru
de50718 to
aaf9349
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/bruno-js/src/variable-list.js (2)
57-59:⚠️ Potential issue | 🟠 Major
get()bypassesfilterKeysexclusion.Reading directly from
this._variablesObj[key]returns keys thatfilterKeysis meant to exclude (e.g.,__name__for environment/globals). Methods likehas(),toObject(), andall()honor the filter via thedataSource, butget()does not, breaking the API contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-js/src/variable-list.js` around lines 57 - 59, The get(key) method currently reads directly from this._variablesObj and bypasses the configured filterKeys; change get to retrieve the value via the same filtered path used by has()/toObject()/all() — e.g. use this._dataSource.get(key) (or run the key through the same filter-check routine that determines exclusion) before passing the value into this._interpolateFn so keys like __name__ are properly excluded; ensure get still returns undefined for excluded keys and keeps interpolation via _interpolateFn.
3-3:⚠️ Potential issue | 🔴 CriticalPrototype pollution risk: regex allows reserved keys.
The regex
/^[\w-.]*$/permits__proto__,constructor, andprototype, creating a prototype pollution vector whenset()directly assigns tothis._variablesObj[key]at line 73. This vulnerability affects all three VariableList instances (variables, environment, globals) since bru.js passes the same regex-basedvalidateKey.Also applies to: 97-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-js/src/variable-list.js` at line 3, The current variableNameRegex (/^[\w-.]*$/) allows reserved property names like "__proto__", "constructor", and "prototype", creating a prototype-pollution risk when VariableList.set() assigns to this._variablesObj[key]; fix by adding an explicit blacklist check in the validation path (e.g., in validateKey or wherever variableNameRegex is used) that rejects reserved keys ("__proto__", "constructor", "prototype") and any keys containing "__proto__" segments before assignment; update VariableList.validateKey / variableNameRegex usage so set(), variables, environment and globals all run this blacklist and throw an error on match rather than writing into this._variablesObj.packages/bruno-js/src/bru.js (1)
72-79:⚠️ Potential issue | 🔴 CriticalPrototype pollution risk in shared
validateKey.The regex
/^[\w-.]*$/allows reserved property names (__proto__,constructor,prototype), enabling prototype mutation throughthis.variables.set(),this.environment.set(), andthis.globals.set(). All three VariableList instances inherit this vulnerability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-js/src/bru.js` around lines 72 - 79, The validateKey function currently allows reserved property names (e.g., "__proto__", "constructor", "prototype") which opens a prototype pollution vector; update validateKey (used by VariableList.set and callers this.variables.set / this.environment.set / this.globals.set) to reject any key that equals or starts with those reserved identifiers by adding an explicit blacklist check (or tighten the regex) to throw on names like "__proto__", "constructor", "prototype" (and any exact matches), and add unit tests to assert those names are rejected. Ensure the change is centralized in validateKey so all VariableList instances inherit the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/bruno-js/src/bru.js`:
- Around line 72-79: The validateKey function currently allows reserved property
names (e.g., "__proto__", "constructor", "prototype") which opens a prototype
pollution vector; update validateKey (used by VariableList.set and callers
this.variables.set / this.environment.set / this.globals.set) to reject any key
that equals or starts with those reserved identifiers by adding an explicit
blacklist check (or tighten the regex) to throw on names like "__proto__",
"constructor", "prototype" (and any exact matches), and add unit tests to assert
those names are rejected. Ensure the change is centralized in validateKey so all
VariableList instances inherit the fix.
In `@packages/bruno-js/src/variable-list.js`:
- Around line 57-59: The get(key) method currently reads directly from
this._variablesObj and bypasses the configured filterKeys; change get to
retrieve the value via the same filtered path used by has()/toObject()/all() —
e.g. use this._dataSource.get(key) (or run the key through the same filter-check
routine that determines exclusion) before passing the value into
this._interpolateFn so keys like __name__ are properly excluded; ensure get
still returns undefined for excluded keys and keeps interpolation via
_interpolateFn.
- Line 3: The current variableNameRegex (/^[\w-.]*$/) allows reserved property
names like "__proto__", "constructor", and "prototype", creating a
prototype-pollution risk when VariableList.set() assigns to
this._variablesObj[key]; fix by adding an explicit blacklist check in the
validation path (e.g., in validateKey or wherever variableNameRegex is used)
that rejects reserved keys ("__proto__", "constructor", "prototype") and any
keys containing "__proto__" segments before assignment; update
VariableList.validateKey / variableNameRegex usage so set(), variables,
environment and globals all run this blacklist and throw an error on match
rather than writing into this._variablesObj.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c1a0089-09a0-4528-92c1-2e45af620301
📒 Files selected for processing (27)
packages/bruno-js/src/bru.jspackages/bruno-js/src/variable-list.jspackages/bruno-js/tests/variable-list.spec.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
✅ Files skipped from review due to trivial changes (15)
- tests/scripting/bru-api/environment/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/variables/folder.bru
- tests/scripting/bru-api/globals/init-user-data/collection-security.json
- tests/scripting/bru-api/globals/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/globals/folder.bru
- tests/scripting/bru-api/environment/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/environment/folder.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.bru
- tests/scripting/bru-api/variables/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/iteration.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru
- packages/bruno-js/tests/variable-list.spec.js
- packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru
- tests/scripting/bru-api/variables/variables.spec.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/bruno-tests/collection/scripting/api/bru/globals/has.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.bru
- tests/scripting/bru-api/environment/environment.spec.ts
- tests/scripting/bru-api/variables/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/variables/iteration.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/get-set.bru
- tests/scripting/bru-api/globals/globals.spec.ts
- packages/bruno-tests/collection/scripting/api/bru/environment/readMethods.bru
…and bru.variables methods This commit modifies the translation logic to replace deprecated Postman API calls with updated bru methods for environment and variable management. The changes include updating method names in both the translation files and the corresponding test cases to ensure consistency and correctness in the translation process.
…slations and implementation This commit comments out the globals.unset and globals.clear methods in the Postman translations and the Bru class implementation, marking them as TODOs to be re-enabled once the UI sync issue is resolved. This change ensures that the code remains functional while addressing the current limitations in the UI.
This commit updates the order of sync read methods in the bru.js shims for variables, environment, and globals to ensure a consistent structure across the code. The changes enhance readability and maintainability without altering functionality.
This commit introduces a new method `getGlobalEnvName` in the Bru class to retrieve the global environment name. Additionally, it updates the Bru shims to expose this method and define a property for the global environment name, enhancing the accessibility of environment information within the application.
…nment variable handling This commit removes the onSet callback from the VariableList class, streamlining the variable setting process. Additionally, it simplifies the environment variable management in the Bru class by eliminating unnecessary logic related to persistent variables, enhancing code clarity and maintainability.
aaf9349 to
7a5c9af
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-converters/src/postman/postman-translations.js`:
- Around line 7-33: The file contains duplicate translation keys that cause the
new namespaced mappings (e.g., 'pm.environment.toObject(',
'pm.environment.clear(', 'pm.variables.toObject(', 'pm.globals.get(',
'pm.globals.set(', 'pm.globals.toObject(') to be overwritten by legacy
translations later in the object; remove the conflicting legacy translations so
the mappings remain pointing to bru.environment.*, bru.variables.*, and
bru.globals.* respectively, and keep any TODO-commented legacy lines disabled
for future re-enable rather than reintroducing duplicate keys.
In `@packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js`:
- Line 108: There is an extra blank line in the property-list-bridge module that
violates the “max one blank line between sections” ESLint rule; open
packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js, find the
stray empty line near the export/module section (around where module.exports or
the PropertyListBridge implementation is defined) and remove the extra blank
line so there is at most one blank line between code sections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c75fffb9-d792-4eb7-adcc-92fe6fe09116
📒 Files selected for processing (45)
packages/bruno-app/src/utils/collections/index.jspackages/bruno-converters/src/postman/postman-translations.jspackages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-converters/tests/postman/postman-translations/postman-comments.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-edge-cases.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-variables.spec.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/combined.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/environment.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/legacy-tests-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/multiline-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/postman-references.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/response.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/testing-framework.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variable-chaining.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variables.test.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.jspackages/bruno-js/src/variable-list.jspackages/bruno-js/tests/variable-list.spec.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
✅ Files skipped from review due to trivial changes (17)
- packages/bruno-tests/collection/scripting/api/bru/globals/folder.bru
- tests/scripting/bru-api/variables/variables.spec.ts
- tests/scripting/bru-api/environment/init-user-data/preferences.json
- tests/scripting/bru-api/environment/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/variables/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/folder.bru
- tests/scripting/bru-api/variables/init-user-data/preferences.json
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variable-chaining.test.js
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/legacy-tests-syntax.test.js
- packages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.bru
- packages/bruno-converters/tests/postman/postman-translations/postman-comments.spec.js
- tests/scripting/bru-api/globals/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/response.test.js
- packages/bruno-tests/collection/scripting/api/bru/environment/iteration.bru
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/testing-framework.test.js
- packages/bruno-js/tests/variable-list.spec.js
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/bruno-converters/tests/postman/postman-translations/postman-edge-cases.spec.js
- tests/scripting/bru-api/globals/init-user-data/collection-security.json
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.js
- tests/scripting/bru-api/globals/globals.spec.ts
- tests/scripting/bru-api/variables/init-user-data/collection-security.json
- packages/bruno-converters/tests/postman/postman-translations/postman-variables.spec.js
- packages/bruno-tests/collection/scripting/api/bru/globals/has.bru
- tests/scripting/bru-api/environment/environment.spec.ts
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/combined.test.js
- packages/bruno-js/src/bru.js
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/multiline-syntax.test.js
- packages/bruno-tests/collection/scripting/api/bru/environment/readMethods.bru
- packages/bruno-app/src/utils/collections/index.js
- packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/postman-references.test.js
- packages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.bru
- packages/bruno-js/src/sandbox/quickjs/shims/bru.js
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variables.test.js
- packages/bruno-converters/src/utils/postman-to-bruno-translator.js
- packages/bruno-tests/collection/scripting/api/bru/variables/folder.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/get-set.bru
- packages/bruno-js/src/variable-list.js
- packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/environment.test.js
- packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru
This commit adds new autocomplete suggestions for `bru.variables`, `bru.environment`, and `bru.globals` methods, enriching the developer experience. Additionally, it refactors the VariableList class to remove inheritance from PropertyList, simplifying its structure and enhancing the API with methods like `has` and `toObject`, which now exclude filtered keys. Tests are updated to reflect these changes and ensure functionality.
7a5c9af to
27c75bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/utils/codemirror/autocomplete.js`:
- Around line 149-169: Update the autocomplete entries for bru.variables,
bru.environment, and bru.globals to include the PropertyList method hints
(matching the pattern used for req.headerList and bru.cookies) by adding method
signatures like .one(), .all(), .each(), .map(), .filter(), .first(), .last(),
.find(), .reduce(), .concat(), .pluck() (and any other PropertyList helpers used
for req.headerList/bru.cookies) alongside the existing
.get/.set/.has/toObject/etc for 'bru.variables' and 'bru.environment'; for
'bru.globals' add the same PropertyList hints but intentionally omit .unset and
.clear as requested. Ensure each added hint follows the exact string format used
in the diff array (e.g., 'bru.variables.one(key)', 'bru.environment.map(fn)',
'bru.globals.each(fn)') so the editor autocomplete recognizes them.
In `@packages/bruno-js/src/variable-list.js`:
- Around line 55-58: The has() method currently uses the `in` operator which
returns true for inherited properties; change it to check only own properties of
`_variablesObj` (e.g., use
`Object.prototype.hasOwnProperty.call(this._variablesObj, key)` or
`Object.hasOwn(this._variablesObj, key)`) while still honoring the `_filterKeys`
exclusion so behavior remains consistent with `toObject()` and `clear()`. Ensure
you update the `has(key)` implementation to use that own-property check
referencing the `has()` method, `_variablesObj`, and `_filterKeys`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61c9df32-fc61-4d7a-ab8b-6a1de3c629c5
📒 Files selected for processing (28)
packages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/src/variable-list.jspackages/bruno-js/tests/variable-list.spec.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
✅ Files skipped from review due to trivial changes (18)
- tests/scripting/bru-api/variables/init-user-data/preferences.json
- tests/scripting/bru-api/environment/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/variables/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/folder.bru
- tests/scripting/bru-api/globals/init-user-data/preferences.json
- tests/scripting/bru-api/variables/variables.spec.ts
- packages/bruno-tests/collection/scripting/api/bru/environment/iteration.bru
- tests/scripting/bru-api/variables/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/has.bru
- tests/scripting/bru-api/environment/init-user-data/collection-security.json
- packages/bruno-js/tests/variable-list.spec.js
- packages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/folder.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru
- tests/scripting/bru-api/environment/environment.spec.ts
- tests/scripting/bru-api/globals/globals.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/scripting/bru-api/globals/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/variables/folder.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/iteration.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru
- packages/bruno-js/src/sandbox/quickjs/shims/bru.js
- packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru
…r improved readability
27c75bf to
31c0cd6
Compare
…List This commit introduces checks in the `set` and `unset` methods of the VariableList class to prevent modification of reserved internal variable names. If a user attempts to set or unset a filtered key, an error is thrown, enhancing the integrity of the variable management system. Corresponding tests are added to ensure the new behavior is correctly implemented.
106a514 to
4f2cd5c
Compare
This commit removes outdated legacy translations from the Postman translations file, which were duplicate keys that conflicted with the namespaced bru.globals, bru.environment, and bru.variables mappings. This change enhances clarity and prevents potential overwrites in the translation logic.
4f2cd5c to
05d0027
Compare
This commit updates the `has` method in the VariableList class to optionally check for a matching value in addition to the key. Corresponding tests are added to verify the new functionality, ensuring accurate behavior when checking for both key existence and value equality.
05d0027 to
4da2780
Compare
…tionality This commit updates the syncWriteMethods in the bru.globals implementation to include 'unset' and 'clear', allowing for more comprehensive manipulation of global variables. This change enhances the flexibility of the global environment management within the application.
4da2780 to
b2a30a2
Compare
…rage
This commit updates the VariableList class to internally manage variables as an array of { key, value } entries, allowing for more flexible input handling. It modifies the constructor to accept either a plain object or an array, and introduces synchronization methods to maintain compatibility with legacy object structures. The `get`, `has`, `set`, `unset`, and `clear` methods are updated to work with the new internal structure, enhancing the overall functionality and maintainability of the variable management system.
This commit introduces additional error handling mechanisms in the bru.js shims, specifically targeting variable and environment management. The changes aim to prevent potential runtime errors and improve the overall stability of the application without affecting existing functionality.
…or cleanup This commit deletes obsolete test files related to bru.environment and bru.variables, streamlining the test suite. Additionally, it updates existing tests to reflect the reduced number of requests and ensure accuracy in the testing framework.
…post-response scripts This commit adds functionality to save and restore environment variables during script execution. It captures the current environment state before clearing variables in the pre-request script and restores them afterward in the post-response script, ensuring that necessary environment settings are maintained throughout the request lifecycle.
…e script This commit simplifies the restoration of environment variables in the post-response script by using a more concise method for iterating over saved variables. The changes enhance code readability while maintaining the functionality of preserving necessary environment settings throughout the request lifecycle.
This commit modifies the API endpoint URLs in multiple test files to replace the placeholder {{host}} with {{localhost}}. This change ensures that the tests point to the correct local server during execution, improving the reliability of the test suite.
b2a30a2 to
5997d30
Compare
JIRA
Summary
Follow-up to #7887. Adds unit tests and E2E test files for the new
bru.variables,bru.environment, andbru.globalsvariable scope APIs, aligned with Postman'sVariableScopeinterface.API surface (per scope)
bru.variablesbru.environmentbru.globals.get(key).set(key, value).has(key).unset(key).clear().toObject().nameVariableListis a standalone class (no PropertyList inheritance) — a clean key-value store matching Postman'spm.environment,pm.variables, andpm.globalsAPI.Key changes
VariableListrewritten as standalone class — removed PropertyList/ReadOnlyPropertyList inheritance, implements only the methods that belong on a variable scopeone,all,idx,count,indexOf,toString, iterators)bru.variables.*,bru.environment.*,bru.globals.*hintspm.*variable scope methods tobru.*equivalentsproperty-list-bridge.js(extra blank line)Test coverage
Unit tests (
packages/bruno-js/tests/variable-list.spec.js):E2E test folders under
packages/bruno-tests/collection/scripting/api/bru/:variables/get-set,has-unset-clearenvironment/get-set(incl.namegetter),has-unset-clear(preserves__name__)globals/get-set,has(incl.namegetter, verifiesunset/clearthrow "not implemented")E2E Playwright specs under
tests/scripting/bru-api/:variables/variables.spec.ts— runs in both developer and safe (QuickJS) modeenvironment/environment.spec.ts— runs in both developer and safe modeglobals/globals.spec.ts— runs in both developer and safe modeContribution Checklist: